Trace type <-> trace module 1-to-1 correspondance #124
Conversation
- the supply-defaults, calc, plot, style and hoverPoints steps each get a file of their own. - helpers convertColumnXYZ, hasColumns and maxRowLength are now modules required into module files.
- colorbar and handleXYDefaults get their own files.
- similar to scatter colorbar
- histogram2d and heatmap share the same calc function for now
- histogram2d and heatmap share the same supplyDefaults step
- so that the new trace modules are registered via Plots.register
|
|
||
| 'use strict'; | ||
|
|
||
| var heatmapHoverPoints = require('../heatmap/hover'); |
There was a problem hiding this comment.
One example of the new pattern:
Here, a contour step requires-in only the part of heatmap it need.
This is important because, browserify bundles everything inside a required module i.e. it doesn't parse through it to check which parts of a module is used.
|
If ok, I would rename the |
| @@ -25,7 +23,8 @@ var histogram = module.exports = {}; | |||
| * constant and % work but they're not so meaningful. I guess it could be cool | |||
| * to allow quadrature combination of errors in summed histograms... | |||
| */ | |||
| Plotly.Plots.register(Plotly.Bars, 'histogram', | |||
|
|
|||
| Plotly.Plots.register(exports, 'histogram', | |||
There was a problem hiding this comment.
in a future PR, these register calls will be moved out of the trace modules indices and into the root index.
There was a problem hiding this comment.
... as discussed in #41 (comment)
Sure. |
|
I'm not going to be able to give this a full review, but overall the changes look great! |
- no more 'composed module' logic.
- split bar style defaults logic into own file to be reused by histogram defaults - split histogram bin defaults logic into own file to be reused by histogram2d and histogram2dcontour defaults
- split up xyz heatmap defaults logic to be reused by contour defaults - split up contour style defaults logic to be resued by histogram2d and histogram2dcontour
- split up histogram2d sample attr logic to be reused in histogram2dcontour defaults - use contour style defaults in histogram2dcontour defaults
- so that they export their own attributes and defaults
- check for 'area' trace type in get-plot-schema routine before calling traceIs()
| if(autocontour) coerce('ncontours'); | ||
| else coerce('contours.size'); | ||
|
|
||
| handleStyleDefaults(traceIn, traceOut, coerce, layout); |
There was a problem hiding this comment.
@alexcjohnson no more Contour.supplyDefaults calling Heatmap.supplyDefaults.
The common part between the two in now in heatmap/xyz_defaults.js.
There was a problem hiding this comment.
composition over inheritance - generally a good pattern :)
|
Looks good to me! ( 💃 as far as I'm concerned) |
|
Ok. I'm going to merge this in. The potential problem I'm most worried about is missing attributes/coerce statements for We'll find out in |
Trace type <-> trace module 1-to-1 correspondance
- so that plotting code - which relies on marker.line.width to set the effective 'bargap' does not error out - broken since #124
- so that plotting code - which relies on marker.line.width to set the effective 'bargap' does not error out - broken since #124
- so that plotting code - which relies on marker.line.width to set the effective 'bargap' does not error out - broken since plotly#124
- so that plotting code - which relies on marker.line.width to set the effective 'bargap' does not error out - broken since #124
@alexcjohnson @mdtusz @cldougl @bpostlethwaite
An important step towards modularity.
This PR:
histogram2dandhistogram2contourhistogramdirectly instead of passing throughBarsheatmap,bars,histogramandcontourinto a digestible one-file-per-step framework.histogram,histogram2d,histogram2dcontourandcontourhave their own attribute object and defaults step instead of relying on composed module logic.